TEL-886: Fix tests and other misc - #792
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| st := NewServiceTest(t, nil) | ||
| call, oc, _ := st.CreateOutboundCall(t) | ||
| serverLocalSDP := oc.cc.LocalSDP() | ||
| serverLocalSDP := getMediaPortRemoteAddr(t, oc.media) |
There was a problem hiding this comment.
🟡 Re-INVITE test for outbound calls compares a network address against an SDP message body, so it can never pass
The outbound re-INVITE check stores the call's RTP destination address instead of the negotiated local SDP (getMediaPortRemoteAddr at pkg/sip/signaling_test.go:723), then compares that address to the SDP returned in the reply, so the check always fails and never verifies the intended behavior.
Impact: The outbound re-INVITE test fails (or silently stops validating anything meaningful), hiding regressions in re-INVITE handling.
Copy-paste mismatch between the address helper and the local-SDP getter
The sibling subtests use the correct source of truth: serverLocalSDP, err := getMediaPort(t, oc.media).GetLocalSDP() (pkg/sip/signaling_test.go:751-752 and pkg/sip/signaling_test.go:766-767). In TestReinvite/outbound/normal, serverLocalSDP is instead a netip.AddrPort returned by getMediaPortRemoteAddr (pkg/sip/signaling_test.go:647-653), and it is then used in require.Equal(t, serverLocalSDP, resp.Body(), "reinvite 200 OK should return server local SDP") (pkg/sip/signaling_test.go:730 and pkg/sip/signaling_test.go:741), comparing an AddrPort with []byte. The preceding require.NotEqual(t, call.localSDP, serverLocalSDP, ...) also becomes trivially true.
| serverLocalSDP := getMediaPortRemoteAddr(t, oc.media) | |
| serverLocalSDP, err := getMediaPort(t, oc.media).GetLocalSDP() | |
| require.NoError(t, err) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| p.audioIn.Close() // Propagate Close() to onwards to room | ||
| p.dtmfIn.Close() // Propagate Close() to onwards to room | ||
| p.audioOut.Close() // Pipeline insulated, but close switch | ||
| p.dtmfOut.Close() // Pipeline insulated, but close switch |
There was a problem hiding this comment.
🟡 Outbound call audio input to the room is closed twice during teardown
The room-bound audio writer is now closed by the media port itself (p.audioIn.Close() at pkg/sip/media_port.go:687) while outbound call teardown still closes the same writer afterwards, so the same audio publisher gets shut down twice.
Impact: Outbound call hangup logs spurious errors and relies on the audio publisher tolerating a second shutdown, which may fail or misbehave.
Teardown path that closes the same chain twice
mediaPort.Close() now propagates Close() through the audioIn/dtmfIn switches to the room-side writers. For outbound calls, audioIn holds exactly c.lkRoomIn (set at pkg/sip/outbound.go:556, possibly wrapped at pkg/sip/outbound.go:554). Teardown calls c.media.Close() (pkg/sip/outbound.go:384) and then c.lkRoomIn.Close() (pkg/sip/outbound.go:393), so the opus encoder / published track writer created in Room.NewParticipantTrack (pkg/sip/room.go:759-779) is closed a second time; the error, if any, is only logged as a warning. Inbound calls do not have this duplicate close (pkg/sip/inbound.go:1620), so the redundant close should be removed from the outbound path (or the writers made idempotent).
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
2 participants
No description provided.